-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-37889: [Java][Doc] Improve JDBC driver documentation #38469
Conversation
|
|
- Correct the user property. - Correct the default for useEncryption. - Elaborate on using the Properties object when connecting and describe the order of precedence. - Mention URI-encoding of option values.
f479807
to
8aa7b18
Compare
- false | ||
- Whether to use TLS (the default is an insecure, plaintext | ||
connection) | ||
- true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe discuss if this is case sensitive? When I was setting up DBeaver, all the other major parameters were in the form FOO_BAR, and this one is documented as fooBar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, all parameters should be camelCase per:
Line 168 in 547b240
public enum ArrowFlightConnectionProperty implements ConnectionProperty { |
and
Line 202 in 547b240
Object value = properties.get(camelName); |
Though the get() implementation seems to also allow fully lower-case. It's probably supposed to be case-insensitive though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's explicitly document whether it's case-sensitive or not, and maybe we can split out a separate ticket to make it case-insensitive if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've documented that names are case-sensitive for simplicity's sake.
If we want to be more precise, names are case-insensitive on the connection URI (since we internally lower-case them after parsing) and case-sensitive in the Properties object, but really they should consistently be case-insensitive.
Other than the one comment above, these fixes are helpful |
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit b42d44e. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
…#38469) ### Rationale for this change Correct inaccuracies and better document how to connect with the Flight SQL JDBC Driver. ### What changes are included in this PR? - Correct the user property. - Correct the default for useEncryption. - Elaborate on using the Properties object when connecting and describe the order of precedence. ### Are these changes tested? N/A ### Are there any user-facing changes? Yes * Closes: apache#37889 Authored-by: James Duong <[email protected]> Signed-off-by: David Li <[email protected]>
…#38469) ### Rationale for this change Correct inaccuracies and better document how to connect with the Flight SQL JDBC Driver. ### What changes are included in this PR? - Correct the user property. - Correct the default for useEncryption. - Elaborate on using the Properties object when connecting and describe the order of precedence. ### Are these changes tested? N/A ### Are there any user-facing changes? Yes * Closes: apache#37889 Authored-by: James Duong <[email protected]> Signed-off-by: David Li <[email protected]>
Rationale for this change
Correct inaccuracies and better document how to connect with the Flight SQL JDBC Driver.
What changes are included in this PR?
Are these changes tested?
N/A
Are there any user-facing changes?
Yes